Introduce new ChannelId struct by optout21 · Pull Request #2485 · lightningdevkit/rust-lightning

optout21 · 2023-08-09T18:42:00Z

Fixes #2408 . Introduces a new ChannelId struct, enclosing the 32-byte data. It has specific constructors, for normal funding-tx-based and for temporary channel ID cases.

ChannelId is exposed as lightning::ln::ChannelId.

This is a breaking change. The type of channel_id parameter has been changed from [u8; 32] to ChannelId in several ChannelManager APIs, namely:

create_chan_between_nodes()
create_chan_between_nodes_with_value()
create_funding_transaction()
sign_funding_transaction()
open_zero_conf_channel()
create_chan_between_nodes_with_value_confirm_second()
create_chan_between_nodes_with_value_confirm()
create_chan_between_nodes_with_value_a()
create_announced_chan_between_nodes()
create_announced_chan_between_nodes_with_value()
close_channel()
test_txn_broadcast()

The type has been changed in several events as well (FundingGenerationReady, PaymentClaimable, PaymentForwarded, ChannelPending, etc.).

Review hints:

The new ChannelId struct is in channel.rs https://github.com/lightningdevkit/rust-lightning/pull/2485/files#diff-8d28e96b6f27edb9a9739b2dce334f8906ec906155d421a7b5b02aa8e6c96057
Affected APIs are in channelmanager.rs https://github.com/lightningdevkit/rust-lightning/pull/2485/files#diff-2423bfc3e50222fe4bfbd77f65cb8bb78625558ace3df2ef80619bc712b634af
Many changes involve logging (log_bytes!(channel_id)), those are straightforward but numerous

TODO:

Determine how it affects APIs, uses, backward-compatibility
Tests for ChannelId serialization/deserialization
~~New macro log_channel_id!()~~
Review hints

codecov-commenter · 2023-08-10T06:10:05Z

Codecov Report

Patch coverage: 84.16% and project coverage change: +0.02% 🎉

Comparison is base (3dffe54) 90.56% compared to head (e99e6ab) 90.59%.
Report is 2 commits behind head on main.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2485      +/-   ##
==========================================
+ Coverage   90.56%   90.59%   +0.02%     
==========================================
  Files         109      110       +1     
  Lines       57304    57410     +106     
  Branches    57304    57410     +106     
==========================================
+ Hits        51899    52008     +109     
+ Misses       5405     5402       -3

Files Changed	Coverage Δ
lightning/src/ln/mod.rs	`96.15% <ø> (ø)`
lightning/src/routing/gossip.rs	`89.94% <0.00%> (-0.13%)`	⬇️
lightning/src/util/test_utils.rs	`73.61% <ø> (ø)`
lightning/src/ln/peer_handler.rs	`61.45% <13.79%> (+0.40%)`	⬆️
lightning/src/util/macro_logger.rs	`93.61% <50.00%> (+4.03%)`	⬆️
lightning/src/events/mod.rs	`48.33% <78.94%> (-0.10%)`	⬇️
lightning/src/ln/msgs.rs	`85.82% <81.66%> (+0.21%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.19% <88.23%> (-0.02%)`	⬇️
lightning/src/ln/channel.rs	`89.83% <95.83%> (+<0.01%)`	⬆️
lightning-invoice/src/utils.rs	`97.67% <100.00%> (ø)`
... and 7 more

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dunxen · 2023-08-10T12:57:29Z

This overall looks like an improvement, but I'd like to maybe see what others had to say about types for different IDs like

temporary channel IDs for V1 unfunded channels
channel IDs for V1 funded channels with known funding txids
temporary channel IDs (revocation basepoint XORed with zero) for V2 channels for open_channel2 and accept_channel2
channel IDs (both party revocation basepoints XORed) for V2 channels after accept_channel2.

For me currently I don't see a huge benefit in splitting all these cases into distinct types or even enum variants. So I think this PR is all we need so that we at least have type safety for channel ID bytes vs hash bytes for example.

TheBlueMatt

Mostly some minor process nits on commit construction, but LGTM.

lightning/src/chain/transaction.rs

lightning/src/ln/channel.rs

lightning-invoice/src/utils.rs

lightning/src/ln/channel.rs

optout21 · 2023-08-22T15:26:25Z

Review comments addressed. commits regrouped (into 2 commits) as suggested.
Only outstanding issue is where to have the ChannelId declaration -- leave in channel.rs, in mod.rs, or in new, own mod.

dunxen

LGTM with moving the ChannelId stuff to its own mod.

lightning/src/ln/channel.rs

optout21 · 2023-08-24T10:11:43Z

Moved ChannelId to its own new module (channel_id.rs); but need to clean up the commits

lightning/src/ln/channel_id.rs

dunxen · 2023-08-24T10:34:20Z

Oops sorry, yeah looks good for squash. Also missing module docs for the new module

optout21 · 2023-08-24T12:20:51Z

Per-commit check failed, fixed (moved last fix in first commit)

optout21 · 2023-08-25T18:09:35Z

This PR touches many logging lines, so it frequently gets in conflict with current main... I keep rebasing+resolving conflicts, but it would be nice to get more approvals ;)
Rebased again.

TheBlueMatt · 2023-08-25T19:09:20Z

Heh, sorry, been a super busy merge week. Will review now and hopefully @dunxen is still around to ack.

TheBlueMatt

Two quick comments, didnt look at the second commit but i assume its fine.

lightning/src/ln/channel_id.rs

optout21 · 2023-08-25T23:33:03Z

Simplified as suggested (@TheBlueMatt): changed struct into a pub tuple; removed .bytes() (use .0 instead).
Also found some instances in tests where temp ID is generated, but from_bytes() was used.
Also some rustdoc formatting.

lightning/src/ln/channel_id.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2023-08-26T00:43:51Z

lightning/src/ln/payment_tests.rs

 use crate::chain::transaction::OutPoint;
 use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
-use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
+use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS};


why did you add brackets?

Hmm, typo, after subsequent add and delete.

Good to keep the blame layer clean (doesn't matter much for use statements), but makes it easier to read stuff back later :)

optout21 · 2023-08-26T06:38:56Z

2 CI builds fail, likely due to #2527 . 2 approvals.

optout21 mentioned this pull request Aug 9, 2023

[WIP] New ChannelId struct #2484

Closed

4 tasks

optout21 marked this pull request as ready for review August 10, 2023 07:54

This was referenced Aug 10, 2023

[WIP] New ChannelId enum #2451

Closed

[WIP] New ChannelId type, wrapper for [u8; 32] #2449

Closed

optout21 changed the title ~~[WIP] Introduce new ChannelId struct~~ Introduce new ChannelId struct Aug 10, 2023

optout21 force-pushed the channel-id-4struct1 branch from 1b1af44 to 46cdcc5 Compare August 11, 2023 19:48

optout21 mentioned this pull request Aug 12, 2023

[minor] Add Display to Payment ID types #2492

Merged

TheBlueMatt reviewed Aug 14, 2023

View reviewed changes

optout21 force-pushed the channel-id-4struct1 branch 2 times, most recently from 50565dc to c84dad1 Compare August 22, 2023 15:23

dunxen reviewed Aug 23, 2023

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

optout21 force-pushed the channel-id-4struct1 branch 2 times, most recently from 31cf609 to 7ec2b2e Compare August 24, 2023 09:38

dunxen previously approved these changes Aug 24, 2023

View reviewed changes

lightning/src/ln/channel_id.rs Outdated Show resolved Hide resolved

optout21 force-pushed the channel-id-4struct1 branch from 5a68b9a to 9b08d92 Compare August 24, 2023 10:42

optout21 dismissed dunxen’s stale review via c89c64e August 24, 2023 10:46

optout21 force-pushed the channel-id-4struct1 branch 2 times, most recently from c89c64e to 5854f40 Compare August 24, 2023 12:20

dunxen previously approved these changes Aug 24, 2023

View reviewed changes

optout21 dismissed dunxen’s stale review via 59fc23b August 25, 2023 18:20

optout21 force-pushed the channel-id-4struct1 branch from 5854f40 to 59fc23b Compare August 25, 2023 18:20

TheBlueMatt reviewed Aug 25, 2023

View reviewed changes

lightning/src/ln/channel_id.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel_id.rs Outdated Show resolved Hide resolved

optout21 added 2 commits August 26, 2023 01:15

Add new ChannelId type; unused

7a992ba

Use new ChannelId type

e99e6ab

optout21 force-pushed the channel-id-4struct1 branch from 59fc23b to e99e6ab Compare August 25, 2023 23:30

TheBlueMatt approved these changes Aug 26, 2023

View reviewed changes

dunxen approved these changes Aug 26, 2023

View reviewed changes

TheBlueMatt merged commit 61d896d into lightningdevkit:main Aug 27, 2023

optout21 mentioned this pull request Aug 27, 2023

Add ChannelId pending change note; formatting #2531

Merged

Conversation

optout21 commented Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dunxen commented Aug 10, 2023

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

optout21 commented Aug 22, 2023

Uh oh!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

optout21 commented Aug 24, 2023

Uh oh!

Uh oh!

dunxen commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

optout21 commented Aug 24, 2023

Uh oh!

optout21 commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Aug 25, 2023

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

optout21 commented Aug 25, 2023

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

optout21 Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Aug 27, 2023

Choose a reason for hiding this comment

Uh oh!

optout21 commented Aug 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

optout21 commented Aug 9, 2023 •

edited

Loading

codecov-commenter commented Aug 10, 2023 •

edited

Loading

dunxen commented Aug 24, 2023 •

edited

Loading

optout21 commented Aug 25, 2023 •

edited

Loading